Migrate comment fragments to Jetpack Compose #11060

Isira-Seneviratne · 2024-05-12T02:58:34Z

What is it?

Bugfix (user facing)
Feature (user facing)
Codebase improvement (dev facing)
Meta improvement to the project (dev facing)

Description of the changes in your PR

Update the Compose dependencies to their latest versions.
Add the Compose library for Coil to handle image loading.
Add the LazyColumnScrollbar library to display scrollbars in Compose.
Migrate the comments to use Jetpack Compose and replace the comment replies fragment with a bottom sheet dialog.
Handle loading the comments and comment replies using the Paging library and a view model.

Before/After Screenshots/Screen Record

Before
Comments	Comment replies

After (initial design)
Comments	Comment replies	Expanded comment replies

After (improved design)
Comments	Comment replies	Expanded comment replies

Screen_recording_20240708_162549.mp4

Relies on the following changes

Migrate image loading from Picasso to Coil #11238

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR. You can find more info and a video demonstration on this wiki page.

Due diligence

I read the contribution guidelines.

The previous implementation was skipping the first page of comments

Stypox · 2024-11-10T23:39:01Z

I pushed a few commits:

redesigned the layouts, see comparison in PR body
merge with refactor to include build fixes by litetex
fix paging skipping first page of comments
handle unknown values without showing -1 in various places
you can test nested replies and unknown comment and like counts in this peertube video: https://share.tube/w/vxu4uTstUBAUromWwXGHrq
you can test hearted and pinned comment in this youtube video: https://www.youtube.com/watch?v=Wf1th3vi53U
~~TODO~~: what should happen when the user clicks on a channel while inside of the comment replies dialog? Currently the channel does open, but only behind the bottom sheet, which is quite strange and difficult to notice. I would propose just lowering the bottom sheet, but not closing it to avoid accidental clicks on channel icons to close the view and forcing the user to scroll back to the original location to continue reading replies.
~~TODO~~: use Icons.Default.* instead of vector icons
~~TODO~~: create utility function to copy to clipboard from inside of a composable function

sonarqubecloud · 2024-11-11T14:33:02Z

Quality Gate passed

Issues
9 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Stypox

Thank you! I implemented the TODOs in my previous comment.

The review comments I left here are things to do in future PRs.

Stypox · 2024-11-11T14:56:21Z

app/src/main/java/org/schabi/newpipe/ui/components/common/DescriptionText.kt

+@Composable
+fun rememberParsedDescription(description: Description): AnnotatedString {
+    // TODO: Handle links and hashtags, Markdown.
+    return remember(description) {
+        if (description.type == Description.HTML) {
+            val styles = TextLinkStyles(SpanStyle(textDecoration = TextDecoration.Underline))
+            AnnotatedString.fromHtml(description.content, styles)
+        } else {
+            AnnotatedString(description.content)
+        }
+    }
+}


Note: reimplementing Description -> AnnotatedString is going to be a tough task, considering how complicated TextLinkifier is: https://github.com/TeamNewPipe/NewPipe/blob/9d6ac67c46a6cc55e03dec0599d8070f7430098d/app/src/main/java/org/schabi/newpipe/util/text/TextLinkifier.java

Stypox · 2024-11-11T15:07:55Z

app/src/main/java/org/schabi/newpipe/ui/components/video/comment/CommentSection.kt

+
+                    is Resource.Error -> {
+                        item {
+                            NoItemsMessage(R.string.error_unable_to_load_comments)


An error panel should be created to handle all of these cases and allow retrying, reporting and opening in browser.

AudricV

Here is some general feedback to be added to my review comments:

the difference between the two loading spinners is a bit disturbing, the new one for comments doesn't seem to match Material Design style;
it would be great if the replies only prevent interacting with comments and not all content (i.e. comment drawers cannot be shown outside of the comments view).

AudricV · 2024-11-11T15:54:07Z

app/src/main/java/org/schabi/newpipe/paging/CommentRepliesSource.kt

+        // params.key is null the first time load() is called, and we need to return the first page
+        val repliesPage = params.key ?: commentInfo.replies
+        val info = withContext(Dispatchers.IO) {
+            CommentsInfo.getMoreItems(service, commentInfo.url, repliesPage)


The app crashes when trying to load comment replies when being offline after comments have been loaded with a network connection at this exact line (it should also happen on comment continuations). This must be fixed.

AudricV · 2024-11-11T15:57:03Z

app/src/main/java/org/schabi/newpipe/paging/CommentRepliesSource.kt

There is no cache in the comment replies fetching, causing the app to reload them each time the device is rotated or a switch between the description content view tab and the comments one is done. This regression must be fixed.

Profpatsch · 2024-11-18T10:53:35Z

I created issues for the remaining post-review comments

Stypox · 2024-11-21T09:45:50Z

Now errors in the comments make the app crash completely (https://soundcloud.com/user-722618400/a-real-playa):

Exception

User Action: ui error
Request: ACRA report
Content Country: GB
Content Language: en-GB
App Language: en_GB
Service: none
Version: 0.27.2
OS: Linux Android 13 - 33

Crash log

java.lang.IllegalArgumentException: Page doesn't contain an URL
	at org.schabi.newpipe.extractor.services.soundcloud.extractors.SoundcloudCommentsExtractor.getPage(SoundcloudCommentsExtractor.java:44)
	at org.schabi.newpipe.extractor.comments.CommentsInfo.getMoreItems(CommentsInfo.java:79)
	at org.schabi.newpipe.paging.CommentsSource$load$info$1.invokeSuspend(CommentsSource.kt:23)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:101)
	at kotlinx.coroutines.internal.LimitedDispatcher$Worker.run(LimitedDispatcher.kt:113)
	at kotlinx.coroutines.scheduling.TaskImpl.run(Tasks.kt:89)
	at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:589)
	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:823)
	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:720)
	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:707)
	Suppressed: kotlinx.coroutines.internal.DiagnosticCoroutineContextException: [LazyStandaloneCoroutine{Cancelling}@16d25a5, Dispatchers.Main.immediate]

Profpatsch · 2024-11-21T10:30:41Z

I’ll add an issue and mark it high priority

github-actions bot added size/medium PRs with less than 250 changed lines size/large PRs with less than 750 changed lines and removed size/medium PRs with less than 250 changed lines labels May 12, 2024

Isira-Seneviratne changed the base branch from dev to refactor May 17, 2024 03:55

Isira-Seneviratne force-pushed the Comments-Compose branch 2 times, most recently from c77ea74 to c106fb0 Compare May 17, 2024 04:02

Isira-Seneviratne marked this pull request as ready for review May 17, 2024 04:48

Isira-Seneviratne force-pushed the Comments-Compose branch from ce7c3ee to 64a92bd Compare May 17, 2024 04:49

github-actions bot added size/giant PRs with more than 750 changed lines and removed size/large PRs with less than 750 changed lines labels Jun 18, 2024

Isira-Seneviratne force-pushed the Comments-Compose branch 5 times, most recently from c834fd0 to 8ee1817 Compare June 19, 2024 12:30

Isira-Seneviratne changed the title ~~Convert comment replies views to Jetpack Compose~~ Migrate comment fragments to Jetpack Compose Jun 21, 2024

Isira-Seneviratne mentioned this pull request Jun 22, 2024

Migrate image loading from Picasso to Coil #11201

Merged

5 tasks

Isira-Seneviratne force-pushed the Comments-Compose branch from da0c0c9 to 7f7faac Compare June 23, 2024 06:03

Isira-Seneviratne force-pushed the Comments-Compose branch 3 times, most recently from 31f1fcf to e973ce1 Compare July 3, 2024 13:28

This was referenced Jul 5, 2024

Migrate image loading from Picasso to Coil #11238

Merged

Migrate playlist fragment to Jetpack Compose #11259

Draft

Isira-Seneviratne force-pushed the Comments-Compose branch from 3ce32b0 to 2049c81 Compare July 11, 2024 01:47

Isira-Seneviratne force-pushed the Comments-Compose branch 5 times, most recently from bace455 to 10dd571 Compare July 28, 2024 02:49

Isira-Seneviratne force-pushed the Comments-Compose branch from e608cc5 to ebcfcc4 Compare August 8, 2024 00:27

Stypox added 7 commits November 10, 2024 16:50

Merge branch 'refactor' into pr11060

bac9f7e

Update compose bom and fix navigation compose without version

36ede24

Improve Comment layout

e6b1341

Improve comment replies dialog layout

802a094

Better handle unknown values for comment & like count

412e1d6

Fix PagingSource for comments

23b3835

The previous implementation was skipping the first page of comments

Fix content color in comment replies fragment

ef56dea

Stypox added 7 commits November 11, 2024 13:34

Slightly improve comment replies header spacing

9d8a79b

Unexpand bottom sheet dialog when clicking on a channel

800961c

Use Icons.Default.* instead of vector assets

a92a285

Add .kotlin/ to gitignore

cea149f

Create utilities to copy to clipboard in Compose code

37d1c78

Show correct reply count in dialog

aea2b7c

Deduplicate and improve Scrollbar theme

fdf36cb

Stypox approved these changes Nov 11, 2024

View reviewed changes

Stypox merged commit 62ab9bd into TeamNewPipe:refactor Nov 11, 2024
6 checks passed

AudricV reviewed Nov 11, 2024

View reviewed changes

Isira-Seneviratne deleted the Comments-Compose branch November 16, 2024 10:17

This was referenced Nov 18, 2024

Handle links and hashtags in DescriptionText.rememberParsedDescription #11715

Open

CommentSection: Add an error panel #11716

Open

CommentRepliesSource: App Crash after being offline #11717

Open

CommentRepliesSource: Add cache for reply fetching #11718

Open

This was referenced Nov 21, 2024

Rewrite: New Comment fragment makes the app crash for comment errors #11728

Open

Soundcloud comments extraction fails for some videos (“Page doesn't contain an URL”) TeamNewPipe/NewPipeExtractor#1243

Open

Stypox mentioned this pull request Nov 27, 2024

Optimize UI #11435

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate comment fragments to Jetpack Compose #11060

Migrate comment fragments to Jetpack Compose #11060

Isira-Seneviratne commented May 12, 2024 •

edited by Stypox

Loading

Stypox commented Nov 10, 2024 •

edited

Loading

sonarqubecloud bot commented Nov 11, 2024

Stypox left a comment

Stypox Nov 11, 2024

Stypox Nov 11, 2024

AudricV left a comment

AudricV Nov 11, 2024

AudricV Nov 11, 2024

Profpatsch commented Nov 18, 2024

Stypox commented Nov 21, 2024 •

edited

Loading

Profpatsch commented Nov 21, 2024

Migrate comment fragments to Jetpack Compose #11060

Migrate comment fragments to Jetpack Compose #11060

Conversation

Isira-Seneviratne commented May 12, 2024 • edited by Stypox Loading

What is it?

Description of the changes in your PR

Before/After Screenshots/Screen Record

Relies on the following changes

APK testing

Due diligence

Stypox commented Nov 10, 2024 • edited Loading

sonarqubecloud bot commented Nov 11, 2024

Quality Gate passed

Stypox left a comment

Choose a reason for hiding this comment

Stypox Nov 11, 2024

Choose a reason for hiding this comment

Stypox Nov 11, 2024

Choose a reason for hiding this comment

AudricV left a comment

Choose a reason for hiding this comment

AudricV Nov 11, 2024

Choose a reason for hiding this comment

AudricV Nov 11, 2024

Choose a reason for hiding this comment

Profpatsch commented Nov 18, 2024

Stypox commented Nov 21, 2024 • edited Loading

Exception

Profpatsch commented Nov 21, 2024

Isira-Seneviratne commented May 12, 2024 •

edited by Stypox

Loading

Stypox commented Nov 10, 2024 •

edited

Loading

Stypox commented Nov 21, 2024 •

edited

Loading